Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add nf-core configs create sub command (attempt 2) #3001

Draft
wants to merge 17 commits into
base: dev
Choose a base branch
from

Conversation

jfy133
Copy link
Member

@jfy133 jfy133 commented May 25, 2024

Closes #2731

Supersedes: https://github.com/nf-core/tools/pull/2852/files#

Unresolved TODOs:

  • Find a way to make TextInput more generic and move to common location (uses a specific ValidateConfig function)
  • Find a way to make ValidateConfig more generic and move to common location (calls CreateConfig)
  • Investigate whether make the TextInput widget not displayed given a particular context (see: https://textual.textualize.io/styles/visibility/), rather than just disable
  • Add check if config file already exists before writing
  • In 2d7863a added issue where unless everything filled in on basic details screen all elements in params dict get sets to none causing failure due to can't NonType + str error when trying to create the file

PR checklist

  • This comment contains a description of changes (with reason)
  • CHANGELOG.md is updated
  • If you've fixed a bug or added code that should be tested, add tests!
  • Documentation in docs is updated

@jfy133 jfy133 marked this pull request as draft May 25, 2024 19:21
Copy link

codecov bot commented May 25, 2024

Codecov Report

Attention: Patch coverage is 7.33591% with 240 lines in your changes missing coverage. Please review.

Project coverage is 73.92%. Comparing base (53a04db) to head (2d7863a).

Current head 2d7863a differs from pull request most recent head 66227b7

Please upload reports for the commit 66227b7 to get more accurate results.

Files Patch % Lines
nf_core/configs/create/utils.py 0.00% 78 Missing ⚠️
nf_core/configs/create/__init__.py 0.00% 41 Missing ⚠️
nf_core/configs/create/basicdetails.py 0.00% 34 Missing ⚠️
nf_core/configs/create/create.py 0.00% 28 Missing ⚠️
nf_core/configs/create/final.py 0.00% 20 Missing ⚠️
nf_core/configs/create/configtype.py 0.00% 15 Missing ⚠️
nf_core/configs/create/welcome.py 0.00% 13 Missing ⚠️
nf_core/__main__.py 33.33% 10 Missing ⚠️
nf_core/configs/__init__.py 0.00% 1 Missing ⚠️
Additional details and impacted files

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jfy133
Copy link
Member Author

jfy133 commented Jun 30, 2024

Two now identified problems:

Major

  • [ ]Problem
    • Failed fields with validation does not block continuing with dialogue screens (i.e. pressing next)
    • Because validation has failed, the tool 'fails' to run CreateConfig with remaining fields (i.e., the resulting object is still created, but 'force' sets all entries to None)
    • Because we can proceed after pressing 'next', we can get to to the end of the questions and tool attempts to write a file. However as config name is now set to None , it fails with a can't None + str error.

Probably due to bad classmethods in utils.py

  • Preferred behavior:
    • Validation only occurs if something filled in. If nothing filled in, returns a valid 'no info' entry (which can be subsequently ignored)
    • If validation fails, block next button (unless again reset to empty, or filled in with valid information)

Minor:

  • Description always gets written "" even though it should not be written (if description != "" or not None:)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant